-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/inline attachments #137
base: master
Are you sure you want to change the base?
Feature/inline attachments #137
Conversation
- provide functions for detecting inline attachments - implement a fast concatenation of a list of parts to a message - add helper method to separate parts into text/inline/attachment - wrap body and inline attachments in `multipart/related`
Hi there, First, thanks for this library. I'm trying this branch, and it works ok; I'm testing it with a set of .eml files, and it is parsed correctly. the only thing that is missing in this PR is the it is a little change though: def get_attachments(%Mail.Message{} = message) do
walk_parts([message], {:cont, []}, fn message, acc ->
case Mail.Message.is_any_attachment?(message) do
true ->
##### THIS
case Mail.Message.get_header(message, :content_disposition) do
["inline", {"filename", filename}] ->
{:cont, List.insert_at(acc, -1, {filename, message.body})}
["attachment", {"filename", filename}] ->
{:cont, List.insert_at(acc, -1, {filename, message.body})}
end
######
false ->
{:cont, acc}
end
end)
|> elem(1)
end |
@jarrodmoldrich thank you very much apoligies this went stale. I know it's a bit rediculous to ask after years but if you could just add regression tests for the additional behavior added I can merge this in. |
+1 |
Sorry, I've been off the radar, but I'm back! I'll have a look at this in the coming weeks. |
+1 |
@bcardarella Sorry for the delay. Added some tests. Let me know if you think this needs anything else. |
Hi @bcardarella do you think this one can go in now? |
that'lll be @andrewtimberlake's call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this.
I’ve added a few comments/questions and a request for some tests to address my concerns.
@@ -251,30 +251,83 @@ defmodule Mail.Renderers.RFC2822 do | |||
|> String.pad_leading(2, "0") | |||
end | |||
|
|||
defp reorganize(%Mail.Message{multipart: true} = message) do | |||
defp split_attachment_parts(message) do | |||
Enum.reduce(message.parts, [[], [], []], fn part, [texts, mixed, inlines] -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not be better to work with a tuple of lists {[], [], []}
to be more explicit about the number of lists being generated?
Enum.filter(message.parts, &match_content_type?(&1, ~r/text\/(plain|html)/)) | ||
|> Enum.sort(&(&1 > &2)) | ||
[text_parts, mixed, inlines] = split_attachment_parts(message) | ||
has_inline = Enum.any?(inlines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might it not be more explicit to use !Enum.empty?(list)
because the behaviour of Enum.any?/1
and Enum.all?/1
are not necessarily intuitive when providing empty lists.
|
||
if has_inline || has_mixed_parts do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need multipart/mixed if there are only inline attachments? Shouldn’t it then just be wrapped in related?
message = Enum.reduce(text_parts, message, &Mail.Message.delete_part(&2, &1)) | ||
|
||
mixed_part = | ||
if has_text_parts do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if it’s only text/html with inline attachments and no text/plain? Then there’s no alternative, right?
} | ||
] | ||
} = message | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need tests for
- only text/html with only inline attachments
- only text/html with only normal attachments
- text/html and text/plain with only inline attachments
- text/html and text/plain with only normal attachments
Changes proposed in this pull request
For parts with
content-dispostion: inline
its important to reorganize themultipart
arrangement in the adapter. In particular it needs to be arrange liked this:My changes reorganize the parts as shown above, while maintaining backwards compatibility. It also adds matching functions for querying for inline attachments as for regular attachments. This change was required for inline images and
.ics
files to work properly, particularly in Outlook webmail.I've stopped short of adding tests to the test suite, and I'll wait for you to validate my approach first. I duplicated this code for
kalys/bamboo_ses
, where it currently functions well for my use cases: kalys/bamboo_ses#43Note: This PR also deprecates #133 as the original order of text parts should be maintained